Skip to content

Change &str input args into impl IntoGStr#1448

Open
jf2048 wants to merge 4 commits intogtk-rs:mainfrom
jf2048:into-gstr
Open

Change &str input args into impl IntoGStr#1448
jf2048 wants to merge 4 commits intogtk-rs:mainfrom
jf2048:into-gstr

Conversation

@jf2048
Copy link
Member

@jf2048 jf2048 commented Mar 1, 2023 •
edited
Loading

Also changes strings with transfer=full to impl Into<GString>

I added new Bounds but not a new Transformation, if anything else requires this stack-callback approach then one could be added

Also added Transformation too to correspond to the new chunk type, so IntoStrV works as well

@sdroege
Copy link
Member

sdroege commented Mar 1, 2023

Same question here, can you create a draft PR with the changes this causes? :)

@sdroege
Copy link
Member

sdroege commented Mar 1, 2023

There's also impl Into<GString> for the transfer-full case and impl IntoStrV for the "array of strings" case that would be useful to consider here.

@jf2048 jf2048 force-pushed the into-gstr branch 2 times, most recently from 59d6829 to 2a45ed4 Compare March 2, 2023 17:47
@sdroege
Copy link
Member

sdroege commented Mar 2, 2023

analysis: Don't use IntoGStr for string params with length

Why not?

@jf2048
Copy link
Member Author

jf2048 commented Mar 2, 2023

I added IntoStrV but I think we want to do gtk-rs/gtk-rs-core#978 first, I don't want to make gir subtract one from len() only to remove that later

@jf2048
Copy link
Member Author

jf2048 commented Mar 2, 2023

Why not?

Those don't need to be nul terminated, it's fine for them to just be &str. I mean, I guess it could be AsRef<str> if we wanted

@sdroege
Copy link
Member

sdroege commented Mar 2, 2023

I added IntoStrV but I think we want to do gtk-rs/gtk-rs-core#978 first, I don't want to make gir subtract one from len() only to remove that later

gtk-rs/gtk-rs-core#1037

Those don't need to be nul terminated, it's fine for them to just be &str. I mean, I guess it could be AsRef<str> if we wanted

Oh right, that makes sense.

@jf2048 jf2048 mentioned this pull request Mar 5, 2023
Open
@sdroege
Copy link
Member

sdroege commented Mar 11, 2023

I think the futures change would be worth its own PR. That can be merged already :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdroege sdroege Awaiting requested review from sdroege

@GuillaumeGomez GuillaumeGomez Awaiting requested review from GuillaumeGomez

@bilelmoussaoui bilelmoussaoui Awaiting requested review from bilelmoussaoui

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jf2048 @sdroege